refactor(error)!: handle errors more gracefully in rustic_core. #202
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
simonsan
added
A-errors
simonsan
commented
Mar 15, 2024
Comment on lines
+122
to
+136
| // REVIEW: Behaviour could have changed here, check! | ||
| index_be | ||
| .into_index() | ||
| .into_index()? | ||
| .into_iter() | ||
| .par_bridge() | ||
| .for_each(|pack| { | ||
| .map(|pack| -> RusticResult<()> { | ||
| let id = pack.id; | ||
| let data = be.read_full(FileType::Pack, &id).unwrap(); | ||
| match check_pack(be, pack, data, &p) { | ||
| Ok(()) => {} | ||
| Err(err) => error!("Error reading pack {id} : {err}",), | ||
| } | ||
| }); | ||
| let data = be | ||
| .read_full(FileType::Pack, &id) | ||
| .map_err(RusticErrorKind::Backend)?; | ||
|
|
||
| check_pack(be, pack, data, &p) | ||
| }) | ||
| .collect::<RusticResult<()>>()?; | ||
|
|
There was a problem hiding this comment.
due to replacing for_each with map (to be able to use results) behaviour could have changed, check again
|
The CI fails for I don't want to implement these tests in this PR though, as it's already quite big. |
simonsan
commented
Mar 15, 2024
crates/core/src/blob/packer.rs
Outdated
Comment on lines
1140
to
1152
| #[rstest] | ||
| #[case(0.5f32, false, "size is too small, should be 'false'")] | ||
| #[case(1.1f32, true, "size is ok, should be 'true'")] | ||
| #[case(1_000_000.0f32, false, "size is too huge: should be 'false'")] | ||
| #[allow(clippy::cast_possible_truncation)] | ||
| fn test_compute_pack_size_ok_passes( | ||
| pack_sizer: PackSizer, | ||
| #[case] input: f32, | ||
| #[case] expected: bool, | ||
| #[case] comment: &str, | ||
| ) -> RusticResult<()> { | ||
| let size_limit = pack_sizer.pack_size()? * 30 / 100; | ||
|
|
||
| let size = (input * size_limit as f32) as u32; | ||
|
|
||
| assert_eq!(pack_sizer.size_ok(size)?, expected, "{comment}"); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Can you think of a better way to test the PackSizer::size_ok() + PackSizer::add_size()?
|
OK, added some more testing, so the coverage test also runs through. 👍🏽 |
simonsan
commented
Mar 15, 2024
aawsome
reviewed
Mar 16, 2024
crates/backend/src/rest.rs
Outdated
Comment on lines
26
to
51
| type Error; | ||
|
|
||
| /// Check a response for an error and treat them as permanent or transient | ||
| fn check_error(self) -> Result<Self, Self::Error> | ||
| where | ||
| Self: Sized; | ||
| } | ||
|
|
||
| impl CheckError for Response { | ||
| type Error = backoff::Error<reqwest::Error>; | ||
|
|
There was a problem hiding this comment.
The latest version improves the name (imho) and this makes it possible to use it further down the line with other types. Also, people can use it easier for their own implementation, e.g. in case they want to use another library at a certain point in time handling the error of a response and backing off. So I like the idea of having a more generalized trait for that.
There was a problem hiding this comment.
/// `ValidateResponse` to add user-defined method `validate` on a response
///
/// This trait is used to add a method `validate` on a response to check for errors
/// and treat them as permanent or transient based on the status code of the response.
///
/// It returns a result with the response if it is not an error, otherwise it returns
/// the associated error.
pub trait ValidateResponse {
/// The error type that is returned if the response is an error
type Error;
/// Check a response for an error and treat it as permanent or transient
///
/// # Errors
///
/// If the response is an error, it will return an error of type [`Self::Error`]
///
/// # Returns
///
/// The response if it is not an error or an error of type [`Self::Error`] if it is an error
fn validate(self) -> Result<Self, Self::Error>
where
Self: Sized;
}| @@ -296,3 +332,57 @@ impl Parent { | |||
| Ok(result) | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
internal remark: Review until here.
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
…r the code base, also handle cli changes with alias Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
…code Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
…ar and keep good style Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
…leanup, add cfg_if for tests and os dependent stuff Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
simonsan
force-pushed
the
feat/remove-unwrap-expect
branch
from
92818c4
to
f1218c8
Compare
simonsan
changed the title
simonsan
added
the
C-zombie
|
Closing this as it's heavily outdated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Till now, rustic_core had a lot of unwraps and expects sprinkled over the code base. This made rustic_core and dependents panic in case of errors. This is unacceptable for a library, hence we fixed it now.
Fixes #140
TODO:
0(*nix) errors left (due to unwrap/expect being denied)Possible Panics left over/reintroduced:
HexId::as_str()/Id:to_hex(): Commit cc6430f4